[Non-Inclusive Language] Replace isDevClusterMaster with isDevClusterManager #1719

manasvinibs · 2022-06-10T22:31:47Z

Signed-off-by: manasvis [email protected]

Description

Coming from opensearch-project/OpenSearch#2589.

OpenSearch repository is going to replace the terminology "master" with "cluster manager".
issue: opensearch-project/OpenSearch#472 (comment), with the plan for its terminology replacement.

In this change we are replacing the instances of the terminology "isDevClusterMaster" with "isDevClusterManager".

Issues Resolved

#1690

Parent #1318

Check List

joshuarrrr · 2022-06-13T17:49:49Z

link checker failure can be ignored as false positive; https://sass-lang.com/blog/libsass-is-deprecated still works fine.

joshuarrrr · 2022-06-13T17:51:41Z

packages/osd-config/src/env.ts

@@ -113,7 +113,7 @@ export class Env {
   * Indicates that this OpenSearch Dashboards  instance is run as development Node Cluster master.


This comment should also be updated to match the new property name.

joshuarrrr · 2022-06-13T17:52:37Z

src/core/server/bootstrap.ts

@@ -82,7 +82,7 @@ export async function bootstrap({
  const env = Env.createDefault(REPO_ROOT, {
    configs,
    cliArgs,
-    isDevClusterMaster: isMaster && cliArgs.dev && features.isClusterModeSupported,


is the update to isMaster covered by a different issue/PR?

Updated! I've added alias to the 'isMaster' referenced from @types/node modules.

Do we need to change or use alias for isMaster? I think it is defined in node cluster index which is not the code we write/control. If it is something we import, do we have to rename?

Reference: https://nodejs.org/docs/latest-v14.x/api/cluster.html#cluster_cluster_ismaster

manasvinibs · 2022-06-13T17:55:28Z

link checker failure can be ignored as false positive; https://sass-lang.com/blog/libsass-is-deprecated still works fine.

Thanks Josh! Where can I find more information on the exact link that has failed the link check?

kavilla · 2022-06-14T07:35:10Z

Thanks for this @manasvinibs,

One immediate feedback would be can we add the details that you add in the PR in the commit details? This is more inclusive on details without having to navigate to the PR.

packages/osd-config/src/env.ts

kavilla

Thanks for this! Please add support for isDevClusterMaster and isDevClusterManager for minor version change. Mark the master one as deprecated and update the commit description to have more information.

ananzh

Thanks @manasvinibs for starting the cleaning up. I agree with Rocky that we need to keep both env.isDevClusterMaster and env.isDevClusterManager in env.options to not break current cx usage. Meanwhile, I think isMaster doesn't need to rename. Because it is a function from node which is not controlled by us and is imported from node cluster index.

import { isMaster } from 'cluster';

ananzh · 2022-06-14T18:46:49Z

src/core/server/bootstrap.ts

@@ -82,7 +82,7 @@ export async function bootstrap({
  const env = Env.createDefault(REPO_ROOT, {
    configs,
    cliArgs,
-    isDevClusterMaster: isMaster && cliArgs.dev && features.isClusterModeSupported,


Do we need to change or use alias for isMaster? I think it is defined in node cluster index which is not the code we write/control. If it is something we import, do we have to rename?

Reference: https://nodejs.org/docs/latest-v14.x/api/cluster.html#cluster_cluster_ismaster

src/core/server/http/http_service.ts

kavilla · 2022-06-15T07:55:52Z

LGTM thanks! Only thing I'm a little bit confused about is the Unverified on the commit. I would just make sure you are signing the commit with the email and name you signed up with Github.

Just another NIT, the commit message exceeds the 50 character limit. But I wouldn't block this PR for that.

joshuarrrr · 2022-06-15T17:19:03Z

packages/osd-config/src/env.ts

    this.isDevClusterMaster = options.isDevClusterMaster;
+    this.isDevClusterManager = options.isDevClusterManager;


I don't want to add too many cooks to this, because this PR is in fine shape, but I think it's worthwhile for us as a team to have a good playbook for deprecating public interfaces, because this isn't the only time we're going to have to do this (and we will still have more PRs for non-inclusive language). But generally the approach I'd favor is

Add the new external interfaces (isDevClusterManager, in this case)

Mark the old interfaces deprecated (isDevClusterMaster)

Make all internal properties read from both, with a priority of the new interface (this.isDevClusterManager = options.isDevClusterManager || options.isDevClusterMaster). That way the fallback logic is not spread throughout the codebase.

Duplicate any existing tests of the old interface for the new one, so that, until we deprecate, both are covered independently. (Depending on the type of deprecation, we may also want to test that the priority order from 3. works correctly if both interfaces are used and they conflict.)

We're already doing steps 1 and 2, so the difference is in 3 and 4. There's an argument that this approach may make the eventual removal/cleanup more difficult, but I think it's a bit of a toss up. Curious for the rest of the team's perspective on the above.

For this purpose of this PR, it would mean changing this line, removing subsequent fallbacks (!(this.coreContext.env.isDevClusterMaster || this.coreContext.env.isDevClusterManager);) and adding additional tests.

My initial thought with this was if I have a new interface field introduced without having to touch the old one, felt more safer option to not break things for the downstream dependencies which might be consuming the old field as it is! But thinking through it and realizing the 'internal' tag on the internal properties, it make sense to go with 3rd point to be more clean and precise. Good call out on point 4 to include tests for the new interface property added. I can update the PR.

For this purpose of this PR, it would mean changing this line, removing subsequent fallbacks (!(this.coreContext.env.isDevClusterMaster || this.coreContext.env.isDevClusterManager);) and adding additional tests.

manasvinibs · 2022-06-16T17:12:32Z

LGTM thanks! Only thing I'm a little bit confused about is the Unverified on the commit. I would just make sure you are signing the commit with the email and name you signed up with Github.

Just another NIT, the commit message exceeds the 50 character limit. But I wouldn't block this PR for that.

Yeah I think it was an issue with conflicting user name used while creating GPG key. I got this one fixed. It should be 'verified' when I update my PR.

ananzh

LGTM

kavilla · 2022-06-17T08:15:00Z

packages/osd-config/src/env.test.ts

    })
  );

  expect(defaultEnv).toMatchSnapshot('env properties');
+  expect(defaultEnv.isDevClusterManager).toBeTruthy();


line 62 says false but this is expected to be truthy this is because isDevClusterMaster is true then we expect isDevClusterManager to be true right? That's great. Can we have a test for the invert. Like isDevClusterManager is set to true. What's the expected behavior?

line 62 says false but this is expected to be truthy this is because isDevClusterMaster is true then we expect isDevClusterManager to be true right? That's great. Can we have a test for the invert. Like isDevClusterManager is set to true. What's the expected behavior?

Updated

…evClusterManager As part of the meta issue opensearch-project/OpenSearch#2589 to track the plan and progress of applying inclusive naming across OpenSearch Repositories In this change we are deprecating the terminology "isDevClusterMaster" and adding "isDevClusterManager" for future usages. Signed-off-by: manasvinibs <[email protected]>

…evClusterManager (#1719) As part of the meta issue opensearch-project/OpenSearch#2589 to track the plan and progress of applying inclusive naming across OpenSearch Repositories In this change we are deprecating the terminology "isDevClusterMaster" and adding "isDevClusterManager" for future usages. Signed-off-by: manasvinibs <[email protected]> (cherry picked from commit 1dda730)

…evClusterManager (#1719) As part of the meta issue opensearch-project/OpenSearch#2589 to track the plan and progress of applying inclusive naming across OpenSearch Repositories In this change we are deprecating the terminology "isDevClusterMaster" and adding "isDevClusterManager" for future usages. Signed-off-by: manasvinibs <[email protected]> Signed-off-by: Manasvini B Suryanarayana <[email protected]> (cherry picked from commit 1dda730)

…evClusterManager (#1719) (#1762) As part of the meta issue opensearch-project/OpenSearch#2589 to track the plan and progress of applying inclusive naming across OpenSearch Repositories In this change we are deprecating the terminology "isDevClusterMaster" and adding "isDevClusterManager" for future usages. Signed-off-by: manasvinibs <[email protected]> Signed-off-by: Manasvini B Suryanarayana <[email protected]> (cherry picked from commit 1dda730) Co-authored-by: Manasvini B Suryanarayana <[email protected]>

…evClusterManager (opensearch-project#1719) (opensearch-project#1762) As part of the meta issue opensearch-project/OpenSearch#2589 to track the plan and progress of applying inclusive naming across OpenSearch Repositories In this change we are deprecating the terminology "isDevClusterMaster" and adding "isDevClusterManager" for future usages. Signed-off-by: manasvinibs <[email protected]> Signed-off-by: Manasvini B Suryanarayana <[email protected]> (cherry picked from commit 1dda730) Co-authored-by: Manasvini B Suryanarayana <[email protected]>

manasvinibs requested a review from a team as a code owner June 10, 2022 22:31

joshuarrrr reviewed Jun 13, 2022

View reviewed changes

manasvinibs force-pushed the Issue-1690 branch 2 times, most recently from 3b9461b to 1fe6799 Compare June 13, 2022 21:11

kavilla linked an issue Jun 14, 2022 that may be closed by this pull request

[Non-Inclusive Language] Replace isDevClusterMaster with isDevClusterManager #1690

Closed

kavilla assigned manasvinibs Jun 14, 2022

kavilla added non-inclusive language v2.1.0 labels Jun 14, 2022

kavilla reviewed Jun 14, 2022

View reviewed changes

packages/osd-config/src/env.ts Show resolved Hide resolved

kavilla requested changes Jun 14, 2022

View reviewed changes

ananzh reviewed Jun 14, 2022

View reviewed changes

manasvinibs force-pushed the Issue-1690 branch from 1fe6799 to a5a9248 Compare June 14, 2022 22:01

manasvinibs requested a review from kavilla June 14, 2022 22:59

manasvinibs force-pushed the Issue-1690 branch from a5a9248 to a32bc41 Compare June 14, 2022 23:46

kavilla previously approved these changes Jun 15, 2022

View reviewed changes

joshuarrrr reviewed Jun 15, 2022

View reviewed changes

kavilla added the backport 2.x label Jun 15, 2022

manasvinibs dismissed kavilla’s stale review via 1d8ec0f June 16, 2022 21:08

manasvinibs force-pushed the Issue-1690 branch from a32bc41 to 1d8ec0f Compare June 16, 2022 21:08

manasvinibs requested review from joshuarrrr and kavilla June 16, 2022 21:13

manasvinibs marked this pull request as draft June 16, 2022 21:30

manasvinibs force-pushed the Issue-1690 branch from 1d8ec0f to a0bdf56 Compare June 16, 2022 22:02

manasvinibs marked this pull request as ready for review June 16, 2022 22:59

ananzh previously approved these changes Jun 17, 2022

View reviewed changes

kavilla reviewed Jun 17, 2022

View reviewed changes

manasvinibs dismissed ananzh’s stale review via 0a6f43c June 17, 2022 17:40

manasvinibs force-pushed the Issue-1690 branch from a0bdf56 to 0a6f43c Compare June 17, 2022 17:40

manasvinibs requested review from kavilla and ananzh June 17, 2022 18:05

kavilla approved these changes Jun 17, 2022

View reviewed changes

ananzh approved these changes Jun 17, 2022

View reviewed changes

joshuarrrr merged commit 1dda730 into opensearch-project:main Jun 17, 2022

opensearch-trigger-bot bot mentioned this pull request Jun 17, 2022

[Backport 2.x] [Non-Inclusive Language] Replace isDevClusterMaster with isDevClusterManager #1762

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Non-Inclusive Language] Replace isDevClusterMaster with isDevClusterManager #1719

[Non-Inclusive Language] Replace isDevClusterMaster with isDevClusterManager #1719

manasvinibs commented Jun 10, 2022 •

edited by kavilla

Loading

joshuarrrr commented Jun 13, 2022

joshuarrrr Jun 13, 2022

joshuarrrr Jun 13, 2022

manasvinibs Jun 13, 2022

ananzh Jun 14, 2022

manasvinibs commented Jun 13, 2022

kavilla commented Jun 14, 2022

kavilla left a comment •

edited

Loading

ananzh left a comment •

edited

Loading

ananzh Jun 14, 2022

kavilla commented Jun 15, 2022 •

edited

Loading

joshuarrrr Jun 15, 2022

manasvinibs Jun 16, 2022 •

edited

Loading

manasvinibs commented Jun 16, 2022

ananzh left a comment

kavilla Jun 17, 2022 •

edited

Loading

manasvinibs Jun 17, 2022

		@@ -113,7 +113,7 @@ export class Env {
		* Indicates that this OpenSearch Dashboards instance is run as development Node Cluster master.

		this.isDevClusterMaster = options.isDevClusterMaster;
		this.isDevClusterManager = options.isDevClusterManager;

[Non-Inclusive Language] Replace isDevClusterMaster with isDevClusterManager #1719

[Non-Inclusive Language] Replace isDevClusterMaster with isDevClusterManager #1719

Conversation

manasvinibs commented Jun 10, 2022 • edited by kavilla Loading

Description

Issues Resolved

Check List

joshuarrrr commented Jun 13, 2022

joshuarrrr Jun 13, 2022

Choose a reason for hiding this comment

joshuarrrr Jun 13, 2022

Choose a reason for hiding this comment

manasvinibs Jun 13, 2022

Choose a reason for hiding this comment

ananzh Jun 14, 2022

Choose a reason for hiding this comment

manasvinibs commented Jun 13, 2022

kavilla commented Jun 14, 2022

kavilla left a comment • edited Loading

Choose a reason for hiding this comment

ananzh left a comment • edited Loading

Choose a reason for hiding this comment

ananzh Jun 14, 2022

Choose a reason for hiding this comment

kavilla commented Jun 15, 2022 • edited Loading

joshuarrrr Jun 15, 2022

Choose a reason for hiding this comment

manasvinibs Jun 16, 2022 • edited Loading

Choose a reason for hiding this comment

manasvinibs commented Jun 16, 2022

ananzh left a comment

Choose a reason for hiding this comment

kavilla Jun 17, 2022 • edited Loading

Choose a reason for hiding this comment

manasvinibs Jun 17, 2022

Choose a reason for hiding this comment

manasvinibs commented Jun 10, 2022 •

edited by kavilla

Loading

kavilla left a comment •

edited

Loading

ananzh left a comment •

edited

Loading

kavilla commented Jun 15, 2022 •

edited

Loading

manasvinibs Jun 16, 2022 •

edited

Loading

kavilla Jun 17, 2022 •

edited

Loading